perf: Optimize ipc stream read performance #24671
Conversation
Liyixin95
requested review from
MarcoGorelli,
alexander-beedie,
c-peters,
orlp,
reswqa and
ritchie46
as code owners
github-actions
bot
added
performance
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24671 +/- ##
==========================================
- Coverage 81.92% 81.90% -0.03%
==========================================
Files 1707 1707
Lines 235483 235453 -30
Branches 3000 3000
==========================================
- Hits 192915 192836 -79
- Misses 41801 41850 +49
Partials 767 767 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| scratch, | ||
| ); | ||
|
|
||
| let new_pos = reader.stream_position()?; |
There was a problem hiding this comment.
Can you make the reborrow of reader explicit here? That is, pass &mut (&mut *reader).take(block_length as u64) instead? Right now this only works because reader.take automatically gets transformed to (&mut *reader).take. I was really confused how this compiled since take takes self and should consume the &mut R.
There was a problem hiding this comment.
I have change this to &mut (&mut *reader).take(block_length as u64). But I still think it's a little verbose...
| .unwrap_or_else(VecDeque::new); | ||
| let mut buffers: VecDeque<arrow_format::ipc::BufferRef> = buffers.iter().collect(); | ||
|
|
||
| // check that the sum of the sizes of all buffers is <= than the size of the file |
There was a problem hiding this comment.
Can you explain why this check was removed?
There was a problem hiding this comment.
I can't easily get file_size after removing the intermediate vec. But remove this check will indeed cause wrong result when file_size and buffer_size not match rather then report an error. Maybe we can change all the subsequent read_to_end to read_excat?
There was a problem hiding this comment.
I can't easily get file_size after removing the intermediate vec. But remove this check will indeed cause wrong result when file_size and buffer_size not match rather then report an error. Maybe we can change all the subsequent
read_to_endtoread_excat?
@orlp kindly pin in case you forget this thread.
There was a problem hiding this comment.
@Liyixin95 I think that should be the way forward then yes. I do want the length to be checked.
There was a problem hiding this comment.
@orlp now all subsequent read_to_end has been change to read_excat. read_excat require vec initial overhead, but I could try to eliminate this using read_buf_excat in another pr, if unstable api is acceptable.
There was a problem hiding this comment.
@Liyixin95 Oh... I hadn't considered that it would add overhead. Could you change back to reserve + take + read_to_end and do a manual assert afterwards that checks the length?
ritchie46
force-pushed
the
main
branch
3 times, most recently
from
90ceb7b to
e9fce55
Compare
|
Thanks, sorry it took a while to review :) |
2 participants
reading a 1.9GB ipc stream file.
before:
peek memory: 3980.86 MB
after:
peek memory: 2086.41 MB